-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve index header metrics #6847
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks
pkg/store/bucket.go
Outdated
@@ -189,9 +189,8 @@ func newBucketStoreMetrics(reg prometheus.Registerer) *bucketStoreMetrics { | |||
m.blockLoadDuration = promauto.With(reg).NewHistogram(prometheus.HistogramOpts{ | |||
Name: "thanos_bucket_store_block_load_duration_seconds", | |||
Help: "The total time taken to load a block in seconds.", | |||
Buckets: []float64{0.1, 0.5, 1, 10, 20, 30, 60, 120}, | |||
Buckets: []float64{0.01, 0.02, 0.05, 0.1, 0.2, 0.5, 1, 2, 5, 15, 30, 60, 90, 120, 300}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking about maybe we can start from a larger number like 1s instead of having multiple small buckets from 0.01 to 0.5.
WDYT? I feel like we don't care much about loading blocks fast. For example, we load block from disk cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same might apply to other two metrics you added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. At least for the thanos_bucket_store_block_load_duration_seconds
and the indexheader_download_duration_seconds
, it may not be reasonable to expect it to take less than 100ms.
But the indexheader_load_duration_seconds
should take much less right? I'll keep the smaller buckets on that metric.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update changelog and mention the new metrics added?
Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com>
Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com>
9407534
to
43bf29c
Compare
Need to sign off DCO |
Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com>
43bf29c
to
bd3c472
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
* Improve binary reader metrics Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com> * Update buckets Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com> * Update CHANGELOG Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com> --------- Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com>
* Improve binary reader metrics Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com> * Update buckets Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com> * Update CHANGELOG Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com> --------- Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com>
Changes
thanos_bucket_store_block_load_duration_seconds
metrics.indexheader_download_duration_seconds
for tracking how long it takes to download the indexheader from objstore.indexheader_load_duration_seconds
to track how long it takes to initialize the binary reader.